Skip to content

Conversation

@pzhuk
Copy link
Contributor

@pzhuk pzhuk commented Apr 10, 2020

This merge has two groups of changes

Admin part:

  • Allow admin removal (addresses Add a menu to demote 💼 Managers #48 )
  • Allow user selection by button or by typping in the username
  • User entity added last_seen attribute to track existing users activity

Image upload part

  • refactor image upload a little bit + handle corner case when list of uploaded images is zero length (caused exceptions in some cases of web-telegram use)
  • fixed typo bug when propery name was edit_products1 instead of edit_products
  • some linting sorts cleanup

@Steffo99 Steffo99 linked an issue Apr 10, 2020 that may be closed by this pull request
@Steffo99 Steffo99 self-requested a review April 10, 2020 21:30
@Steffo99 Steffo99 self-assigned this Apr 10, 2020
@pzhuk
Copy link
Contributor Author

pzhuk commented Apr 11, 2020

sorry for couple of last minute fixes
I suggest to squash this MR if you decide to merge it

@Steffo99
Copy link
Owner

Sorry that I took a while to respond, I was on Easter break :)

I'll check your PR right now 😄

Copy link
Owner

@Steffo99 Steffo99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So many things to check! 😮

Thanks for everything, but there are a few things that need to be changed before I can merge this...

first_name = Column(String, nullable=False)
last_name = Column(String)
username = Column(String)
last_seen = Column(DateTime, default=datetime.datetime.utcnow)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing existing tables... is troublesome, as all current users of the bot would need to migrate somehow their database to the new schema.

Either we provide a migration script, or we put the new info in a new, different table instead... 🤔

I could probably help with that, but it may take a while...

continue
# Return the first capture group
return match.group(1)
return match.group(1) or match.group(2)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is unnecessary and possibly dangerous 🤔 , please revert it!

In your use case, you should be able to structure your regex differently to have a single capture group.
Something like... (?:user_|\@)(.*) should work.

admin.display_on_help = not admin.display_on_help
elif callback.data == "cmd_remove":
self.session.query(db.Admin).filter(db.Admin.user_id==user.user_id).delete()
message = self.bot.send_message(self.chat.id, strings.conversation_confirm_admin_dismissal)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when an user removes themselves from Manager? Does the bot stay without any managers?
What if an user removes another one from managers, but the other still has a conversation open?

@Steffo99 Steffo99 marked this pull request as draft June 9, 2020 00:40
@Steffo99 Steffo99 added the feature A request for a new feature. label Aug 22, 2020
@Steffo99 Steffo99 removed their assignment May 19, 2021
@Steffo99 Steffo99 marked this pull request as ready for review September 4, 2021 21:49
@Steffo99 Steffo99 added the waiting Nothing can be done for this issue except waiting for an external situation to change. label Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature A request for a new feature. waiting Nothing can be done for this issue except waiting for an external situation to change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a menu to demote 💼 Managers

2 participants